Feature/2237 add column name #2246
Conversation
| * @param 'tables'|'views' $nodeCollection Indicates whether to create a row on a table or view | ||
| * @param int $nodeId The identifier of the targeted table or view | ||
| * @param string|array<string, mixed> $data An array containing the column identifiers and their values | ||
| * @return DataResponse<Http::STATUS_OK, TablesRow, array{}>|DataResponse<Http::STATUS_FORBIDDEN|Http::STATUS_BAD_REQUEST|Http::STATUS_NOT_FOUND|Http::STATUS_INTERNAL_SERVER_ERROR, array{message: string}, array{}> |
There was a problem hiding this comment.
The added field should be reflected in the response definition of TablesRow, cf. https://github.com/nextcloud/tables/blob/main/lib/ResponseDefinitions.php#L76-L84
lib/Db/Row2.php
Outdated
| /** | ||
| * return merged response-only metadata into the data cells. | ||
| */ |
There was a problem hiding this comment.
The method names says this already, no need to duplicate :)
What we want is however a @return annotation with the array shape. Or @psalm-return as done at jsonSerialize(), which itself would need to be adjusted afterwards.
Did you double check, cannot we only extend jsonSerialize() instead of adding a similar method?
There was a problem hiding this comment.
i was a bit hesitant on using 'jsonSerialize' as i thought i might interfere in another place, but did more testing and it's fine.
lib/Db/Row2.php
Outdated
| private ?string $lastEditBy = null; | ||
| private ?string $lastEditAt = null; | ||
| private ?array $data = []; | ||
| private array $cellMeta = []; |
There was a problem hiding this comment.
Should need to be added to a VIRTUAL_PROPERTIES const, otherwise the Mapper will complain. Have a look at https://github.com/nextcloud/tables/blob/main/lib/Db/Column.php#L161C2-L161C114 for comparison.
Also, the name is too broad and not specific (same problem with data already…), but right now I am also missing a better suggestion.
There was a problem hiding this comment.
ill rename it cellMetadata as the array holds metadata for each cell.
lib/Service/RowService.php
Outdated
| $fullRowData = []; | ||
| $columnNames = []; | ||
|
|
||
| foreach ($columns as $c) { |
There was a problem hiding this comment.
| foreach ($columns as $c) { | |
| foreach ($columns as $column) { |
Please don't save bytes where it is not necessary :)
lib/Service/RowService.php
Outdated
|
|
||
| $rows = $data instanceof RowDataInput ? iterator_to_array($data) : $data; | ||
|
|
||
| foreach ($rows as $r) { |
lib/Service/RowService.php
Outdated
| // attached columnname to returned row for the response only | ||
| foreach ($fullRowData as $meta) { | ||
| if (isset($meta['columnId']) && array_key_exists('columnName', $meta)) { | ||
| $insertedRow->addCellMeta((int)$meta['columnId'], ['columnName' => $meta['columnName']]); |
There was a problem hiding this comment.
can we generate it in Row2 instead? data is populated there. Having all logic in Row2 would capsulate it there and not leak logic to other classes around.
This PR is a fix for #1840 and #2237.